OCPBUGS-11933: Ignore informative annotations upon admission #117

liouk · 2023-07-17T13:36:01Z

This PR introduces a list of known informative annotations; pod admission then will ignore any pod mutations that only add these annotations.

The goal is to prevent a pod to be re-admitted and potentially change the SCC under which it gets admitted when a mutation only adds informative annotations, which shouldn't cause SCC changes.

This also fixes a bug in the TestShouldIgnore unit test within pkg/securitycontextconstraints/sccadmission/admission_test.go.

openshift-ci-robot · 2023-07-17T13:36:06Z

@liouk: This pull request references Jira Issue OCPBUGS-11933, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This PR introduces a list of known informative annotations; pod admission then will ignore any pod mutations that only add these annotations.

The goal is to prevent a pod to be re-admitted and potentially change the SCC under which it gets admitted when a mutation only adds informative annotations, which shouldn't cause SCC changes.

This also fixes a bug in the TestShouldIgnore unit test within pkg/securitycontextconstraints/sccadmission/admission_test.go.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

liouk · 2023-07-17T13:40:02Z

/jira refresh

openshift-ci-robot · 2023-07-17T13:40:08Z

@liouk: This pull request references Jira Issue OCPBUGS-11933, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @xingxingxia

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/securitycontextconstraints/sccadmission/admission.go

liouk · 2023-08-24T14:22:38Z

/hold
contains fixups

liouk · 2023-08-31T13:34:29Z

/hold
holding until 4.14 stabilized or branched out

stlaz · 2023-09-07T11:36:08Z

please create a proof PR in openshift/kubernetes

liouk · 2023-09-07T12:55:15Z

/hold cancel
fixups removed
the automation should be capable of merging this when the critical-fixes-only gate is lifted

liouk · 2023-10-05T14:27:13Z

please create a proof PR in openshift/kubernetes

@stlaz this is the poc PR: openshift/kubernetes#1708

pkg/securitycontextconstraints/sccadmission/admission.go

stlaz · 2023-10-09T12:57:42Z

/approve
we may want to ask @deads2k to have a peek but the PR seems ok to me

deads2k · 2023-11-13T19:46:58Z

pkg/securitycontextconstraints/sccadmission/admission.go

+			return false, admission.NewForbidden(a, fmt.Errorf("object was marked as kind pod but was unable to be converted: %v", a.GetOldObject()))
+		}
+
+		// never ignore any spec changes


once vertical scaling is a thing, we will ignore some. Just be ready.

is #128 legit, too?

pkg/securitycontextconstraints/sccadmission/admission.go

deads2k · 2023-11-16T15:46:24Z

pkg/securitycontextconstraints/sccadmission/admission.go

+
+	// do not consider annotations for further comparisons, they were checked above
+	origNewAnnotations := newPod.ObjectMeta.Annotations
+	newPod.ObjectMeta.Annotations = oldPod.ObjectMeta.Annotations


Don't mutate inputs. This got us into trouble in validation a couple years back. This isn't needed to make the mutatingGCFields check work.

You're right -- I wanted to avoid another deep copy (as the IsMutatingGCFields check does one). I did this now by overriding the equalities, and in particular for the map[string]string type (which is the type of Annotations).

As discussed with @stlaz, opted for doing an object deep-copy and modifying the annotations of the copy in order to perform the controller GCFields check.

…pon admission

openshift-ci · 2024-02-05T15:55:40Z

@liouk: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

stlaz · 2024-03-13T13:06:39Z

/lgtm

openshift-ci · 2024-03-13T13:07:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liouk, stlaz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/securitycontextconstraints/OWNERS~~ [stlaz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2024-03-13T13:12:02Z

@liouk: Jira Issue OCPBUGS-11933: All pull requests linked via external trackers have merged:

openshift/apiserver-library-go#117

Jira Issue OCPBUGS-11933 has been moved to the MODIFIED state.

In response to this:

This PR introduces a list of known informative annotations; pod admission then will ignore any pod mutations that only add these annotations.

The goal is to prevent a pod to be re-admitted and potentially change the SCC under which it gets admitted when a mutation only adds informative annotations, which shouldn't cause SCC changes.

This also fixes a bug in the TestShouldIgnore unit test within pkg/securitycontextconstraints/sccadmission/admission_test.go.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-merge-robot · 2024-04-13T14:00:06Z

Fix included in accepted release 4.16.0-0.nightly-2024-04-13-070448

openshift-ci bot requested review from ibihim and s-urbaniak July 17, 2023 13:36

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jul 17, 2023

openshift-ci bot requested a review from xingxingxia July 17, 2023 13:40

liouk changed the title ~~OCPBUGS-11933: Ignore informative annotations upon admission~~ WIP: OCPBUGS-11933: Ignore informative annotations upon admission Jul 17, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 17, 2023

liouk force-pushed the admission-informative-annotations branch 3 times, most recently from 4b9741c to d892ff7 Compare July 24, 2023 14:12

liouk changed the title ~~WIP: OCPBUGS-11933: Ignore informative annotations upon admission~~ OCPBUGS-11933: Ignore informative annotations upon admission Jul 25, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 25, 2023

stlaz reviewed Jul 26, 2023

View reviewed changes

pkg/securitycontextconstraints/sccadmission/admission.go Show resolved Hide resolved

liouk changed the title ~~OCPBUGS-11933: Ignore informative annotations upon admission~~ WIP: OCPBUGS-11933: Ignore informative annotations upon admission Jul 28, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2023

liouk force-pushed the admission-informative-annotations branch from d892ff7 to 9a75644 Compare July 28, 2023 10:24

liouk changed the title ~~WIP: OCPBUGS-11933: Ignore informative annotations upon admission~~ OCPBUGS-11933: Ignore informative annotations upon admission Jul 28, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2023

stlaz reviewed Aug 17, 2023

View reviewed changes

pkg/securitycontextconstraints/sccadmission/admission.go Outdated Show resolved Hide resolved

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2023

liouk force-pushed the admission-informative-annotations branch from dbffacb to 9836a75 Compare September 7, 2023 12:53

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2023

liouk force-pushed the admission-informative-annotations branch from 9836a75 to 077f27f Compare September 7, 2023 12:56

liouk mentioned this pull request Sep 18, 2023

DO NOT MERGE: POC: SCC admission informative annotations openshift/kubernetes#1708

Closed

pkg/scc/sccadmission: fix argument order, new object comes first

4e48eea

liouk force-pushed the admission-informative-annotations branch from 077f27f to 7385c01 Compare September 18, 2023 13:14

stlaz reviewed Oct 9, 2023

View reviewed changes

pkg/securitycontextconstraints/sccadmission/admission.go Outdated Show resolved Hide resolved

stlaz reviewed Oct 9, 2023

View reviewed changes

pkg/securitycontextconstraints/sccadmission/admission.go Show resolved Hide resolved

liouk force-pushed the admission-informative-annotations branch from 7385c01 to 593b8dc Compare October 16, 2023 09:10

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2023

liouk force-pushed the admission-informative-annotations branch from 593b8dc to a0fc30b Compare October 16, 2023 09:21

deads2k reviewed Nov 13, 2023

View reviewed changes

pkg/securitycontextconstraints/sccadmission/admission.go Outdated Show resolved Hide resolved

liouk force-pushed the admission-informative-annotations branch from a0fc30b to def0bab Compare November 16, 2023 13:21

deads2k reviewed Nov 16, 2023

View reviewed changes

liouk force-pushed the admission-informative-annotations branch from def0bab to 08aef77 Compare January 25, 2024 13:46

pkg/scc/sccadmission: ignore known informative annotation additions u…

8e50815

…pon admission

liouk force-pushed the admission-informative-annotations branch from 08aef77 to 8e50815 Compare February 5, 2024 15:47

openshift-ci bot assigned stlaz Mar 13, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2024

openshift-merge-bot bot merged commit facc40c into openshift:master Mar 13, 2024

openshift-ci-robot mentioned this pull request Apr 11, 2024

OCPBUGS-11933: UPSTREAM: <drop>: bump apiserver-library-go openshift/kubernetes#1910

Merged

OCPBUGS-11933: Ignore informative annotations upon admission #117

OCPBUGS-11933: Ignore informative annotations upon admission #117

Uh oh!

Conversation

liouk commented Jul 17, 2023

Uh oh!

openshift-ci-robot commented Jul 17, 2023

Uh oh!

liouk commented Jul 17, 2023

Uh oh!

openshift-ci-robot commented Jul 17, 2023

Uh oh!

Uh oh!

Uh oh!

liouk commented Aug 24, 2023

Uh oh!

liouk commented Aug 31, 2023

Uh oh!

stlaz commented Sep 7, 2023

Uh oh!

liouk commented Sep 7, 2023

Uh oh!

liouk commented Oct 5, 2023

Uh oh!

Uh oh!

Uh oh!

stlaz commented Oct 9, 2023

Uh oh!

deads2k Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

stlaz Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

deads2k Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

liouk Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

liouk Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Feb 5, 2024

Uh oh!

stlaz commented Mar 13, 2024

Uh oh!

openshift-ci bot commented Mar 13, 2024

Uh oh!

openshift-ci-robot commented Mar 13, 2024

Uh oh!

openshift-merge-robot commented Apr 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants